Make the notary version detectable by LaurentGoderre · Pull Request #37 · docker/notary-official-images

LaurentGoderre · 2024-07-11T15:52:01Z

Syft is able to use the ldflags from go to properly detect the main version:

https://github.com/anchore/syft/blob/main/syft/pkg/cataloger/golang/parse_go_binary_test.go#L1034-L1039

In order to make this work, we would need to enable that feature in the Scout Scanner

tianon-sso

We'll also probably need to bump the Alpine version 😭

whalelines

Can the rationale for moving from an intermediate image to a two-stage build be given in the PR description?

whalelines · 2024-08-07T16:18:46Z

-      - run: |
-          docker build notary-builder --tag notary:builder
-          tag="$(docker run --rm notary:builder sh -c 'echo $TAG' | awk '{gsub(/^v/, ""); print}')"
-          docker tag notary:builder "notary:${tag}-builder"


Why is the builder variant being removed?

docker-library/official-images#15568 (comment)

whalelines · 2024-08-07T16:21:09Z

@@ -1,3 +1,31 @@
+FROM golang:1.19-alpine{{ .alpine }} AS build


Perhaps #36 should just be closed and its changes included in this PR?

whalelines · 2024-08-07T16:24:32Z

+WORKDIR /go/src/$NOTARYPKG
+RUN set -eux; \
+	git clone -b "$TAG" --depth 1 "https://$NOTARYPKG.git" .; \
+# In case the version in file doens't match the tag (like in 0.7.0)


Suggested change

# In case the version in file doens't match the tag (like in 0.7.0)

# In case the version in file doesn't match the tag (like in 0.7.0)

What does this mean?

When version 0.7.0 was release, the main file used in the code to determine the version wasn't bumped.

This is the 0.7.0 commit where the version is wrong.

https://github.com/notaryproject/notary/blob/b0b6bfdd4933081e8d5ae026b24e8337311dd598/NOTARY_VERSION

whalelines · 2024-08-07T18:10:42Z

+
+RUN apk add --no-cache git make
+
+ENV NOTARYPKG github.com/theupdateframework/notary


Suggested change

ENV NOTARYPKG github.com/theupdateframework/notary

ENV NOTARYPKG github.com/notaryproject/notary

This must match what is in the go manifest which in version 0.7.0 is still that one as per:

https://github.com/notaryproject/notary/blob/b0b6bfdd4933081e8d5ae026b24e8337311dd598/go.mod#L1

They are the same repo, theupdateframework/notary redirects to notaryproject/notary, as your link shows.

Right, but this is used in the Go modules, so it has to match what Go thinks the module name is 🙃

Exactly! It's confusing I know

Careful; the module wasn't renamed yet in the current release though (still uses github.com/theupdateframework/notary); https://github.com/notaryproject/notary/blob/v0.7.0/go.mod#L1

i.e. github.com/notaryproject/notary did not yet do a release

whalelines · 2024-08-07T18:18:05Z

+	go mod vendor; \
+# TODO remove for the next release of Notary (which should include efc35b02698644af16f6049c7b585697352451b8 & ca095023296d2d710ad9c6dec019397d46bf8576)
+# Make the version detectable by scanners
+	sed -i -r -E 's|(version.NotaryVersion=\$\(NOTARY_VERSION\))|\1 -X $(NOTARY_PKG)/version.Version=$(NOTARY_VERSION)|' Makefile; \


Suggested change

sed -i -r -E 's|(version.NotaryVersion=\$$NOTARY_VERSION$)|\1 -X $(NOTARY_PKG)/version.Version=$(NOTARY_VERSION)|' Makefile; \

sed -i -E 's,(version\.NotaryVersion=\$$NOTARY_VERSION$),\1 -X $(NOTARY_PKG)/version.Version=$(NOTARY_VERSION),' Makefile; \

-r and -E are redundant, -E is more common.

I'd avoid using a regular expression special character, | in this case, as the substitution delimiter.

Why not the special character? I've seen it done like this many time

How would you use the special character? How would you escape it?

I usually use -r, but -E is POSIX:

-E, -r, --regexp-extended use extended regular expressions in the script (for portability use POSIX -E).

Perhaps you meant -e, which is otherwise implied?

[-e script] [--expression=script] ... [script-if-no-other-script] ... -e script, --expression=script add the script to the commands to be executed

I usually use @ or ! as my "non-/ delimiter", but I don't think the exact choice matters that much here, right? If we want to use | in our regex in the future, it's pretty easy to swap then. 🤷

LaurentGoderre · 2024-08-07T19:02:06Z

We can undo those change when this lands: notaryproject/notary#1704

jonnystoten

❤️

LaurentGoderre force-pushed the version-auto-detect branch from 2649822 to 8dedc9a Compare July 11, 2024 15:53

LaurentGoderre marked this pull request as ready for review July 23, 2024 20:29

LaurentGoderre marked this pull request as draft July 23, 2024 20:29

Make the notary version detectable

dbdbf02

LaurentGoderre force-pushed the version-auto-detect branch from 8dedc9a to dbdbf02 Compare August 6, 2024 20:46

LaurentGoderre marked this pull request as ready for review August 6, 2024 20:48

Removed the builder variant

cf64d96

LaurentGoderre force-pushed the version-auto-detect branch from e0ce045 to cf64d96 Compare August 7, 2024 13:56

tianon-sso approved these changes Aug 7, 2024

View reviewed changes

whalelines reviewed Aug 7, 2024

View reviewed changes

jonnystoten approved these changes Aug 23, 2024

View reviewed changes

jonnystoten merged commit 4fc4c25 into docker:master Aug 23, 2024

LaurentGoderre deleted the version-auto-detect branch August 26, 2024 13:51

		@@ -1,3 +1,31 @@
		FROM golang:1.19-alpine{{ .alpine }} AS build

	# In case the version in file doens't match the tag (like in 0.7.0)
	# In case the version in file doesn't match the tag (like in 0.7.0)


		RUN apk add --no-cache git make

		ENV NOTARYPKG github.com/theupdateframework/notary

	ENV NOTARYPKG github.com/theupdateframework/notary
	ENV NOTARYPKG github.com/notaryproject/notary

	sed -i -r -E 's\|(version.NotaryVersion=\$\(NOTARY_VERSION\))\|\1 -X $(NOTARY_PKG)/version.Version=$(NOTARY_VERSION)\|' Makefile; \
	sed -i -E 's,(version\.NotaryVersion=\$\(NOTARY_VERSION\)),\1 -X $(NOTARY_PKG)/version.Version=$(NOTARY_VERSION),' Makefile; \

Uh oh!

Conversation

LaurentGoderre commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tianon-sso left a comment

Choose a reason for hiding this comment

Uh oh!

whalelines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaurentGoderre commented Aug 7, 2024

Uh oh!

jonnystoten left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LaurentGoderre commented Jul 11, 2024 •

edited

Loading